-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Discover] Improve HTML formatting of fields with a list of values #136684
[Discover] Improve HTML formatting of fields with a list of values #136684
Conversation
6731fa3
to
e6c388d
Compare
e6c388d
to
ebebb66
Compare
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, this is a nice little big improvement , thank you 🙏 ... tested a-la-carte and works as expected. Just one note, the text in the PR doesn't mention what was changed (like added fancy brackets). this could be added
Good call, summary updated to include changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a neat edge case @Dosant , also do agree it shouldn't block, but it's good to be aware of this 👍 . BTW there are no plans to add React based formatters which would e.g. allow expand/collapse such a list in UI, are there? |
@elasticmachine merge upstream |
There are no plans, but there is this bug in the backlog: #75729, which I think could be fixed with some kind of React-based formatter where internally it updates with an interval Another use-case I had in mind is that we possibly could use lazy-loading inside such React-based formatter and stop loading some of those heavy formatting functions into the initial bundle with all the formatters I think it is worth at least creating an issue even if this won't be escalated soon |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: cc @davismcphee |
Oh yeah, good catch. I guess to the very keen eye the comma colours could help, but definitely not ideal. This will be good to keep in mind if we later go the React direction. |
Summary
This PR improves HTML formatting of fields with a list of values to help differentiate between long numbers and numeric arrays. These changes were made:
Discover grid:
Expanded document:
Resolves #135357.
Checklist
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsAny UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listFor maintainers